RichText: Fix empty value delete behaviors #8735

aduth · 2018-08-08T22:54:43Z

This pull request seeks to resolve two issues related to deletion behaviors on empty RichText (paragraph blocks). The first is described in #8731, the second I'm not sure has yet been surfaced (and exists in the production v3.4 release):

Navigate to Posts > Add New
Click writing prompt
Press Enter twice
Click first paragraph
Press (forward) Delete
Observe two paragraph deletion instead of one

The latter of these is due to the fact that we did not distinguish between onMerge and onRemove in the key handlers of RichText, thus causing both to fire, merging into the one block and then deleting that one block, resulting in the double-deletion. This has been accounted for here by updating the block's onMerge to ~~return a boolean indicating whether it was successful in its merge. This is interpreted by RichText to determine whether a removal is warranted, if possible~~ only handle removal on the Backspace key (see #8735 (comment)).

The issue causing #8731 is a result of the fact that when focused in an empty RichText, the caret is placed adjacent (before) a "bogus" br node inserted by TinyMCE. The implementation of the general purpose isHorizontalEdge interprets this to mean that the caret is not at the end of the field, thus it would never allow the removal.

Implementation notes:

I adopted waitForRichTextInitialization from #8306 because I was having issues with failures locally without it. I'm not terribly happy with its inclusion, also because it only works for explicit inserts of the paragraph block, not a new paragraph inserted via Enter or other keyboard interactions.

Testing instructions:

Repeat testing instructions from #8731 and the above second set of issue steps, verifying that paragraph merges as expected, with a maximum of one removal.

Ensure end-to-end tests pass:

npm run test-e2e

aduth · 2018-08-08T23:26:21Z

The failing end-to-end test is legitimate, and indicates that we rely on RichText to remove itself even when calling onMerge and a block does exist in the direction of the delete (before / after), but it's not mergeable. For example, an empty paragraph following an image block, when pressing Backspace, will both merge and remove (in master), and thus fails with the changes as proposed here.

We seem to be quite inconsistent in how we expect this to work. It doesn't seem like we should want both a merge and remove to happen, but rather one or the other, depending on whether a merge is possible and if the current RichText is empty.

Some ideas off the top of my head:

Somehow account for "mergeable blocks" in the newly-introduced boolean return value of onMerge here
When RichText is empty and delete is pressed, don't try to merge at all, just remove, and somehow trigger the appropriate caret movement to the next / previous block
Handle removal from BlockListBlock when no next / previous block

tofumatt

The code here makes sense but I'm slightly too tired to offer much more input on
#8735 (comment)

I'll just leave my comments here. 🤷‍♂️

tofumatt · 2018-08-09T00:15:38Z

packages/editor/src/components/rich-text/README.md

@@ -41,11 +41,11 @@ Render a rich [`contenteditable` input](https://developer.mozilla.org/en-US/docs

 ### `onMerge( forward: Boolean ): Function`

-*Optional.* Called when blocks can be merged. `forward` is true when merging with the next block, false when merging with the previous block.
+*Optional.* Called when blocks can be merged. `forward` is true when merging with the next block, false when merging with the previous block. The callback should return `true` if a merge is possible, or `false` otherwise. This is used in determining whether an empty field should be deleted instead of merged.


Just a drive-by review nitpick: "true" should be backtick-wrapped (eg "true") for improved readability (it's done that way elsewhere).

Changes backed out in rebased d29d5d1.

tofumatt · 2018-08-09T00:15:53Z

packages/editor/src/components/rich-text/README.md


 ### `onRemove( forward: Boolean ): Function`

-*Optional.* Called when the block can be removed. `forward` is true when the selection is expected to move to the next block, false to the previous block.
+*Optional.* Called when the block can be removed. `forward` is true when the selection is expected to move to the next block, false to the previous block. Called when pressing Backspace or Delete on an empty field and only when `onMerge` explicitly returns `false` to indicate non-merge.


Same here re: true and false.

Changes backed out in rebased d29d5d1.

tofumatt · 2018-08-09T00:16:26Z

packages/editor/src/components/rich-text/index.js

@@ -426,41 +427,84 @@ export class RichText extends Component {
 	}

 	/**
-	 * Handles a keydown event from TinyMCE.
+	 * Handles a delete key down event to handle merge or removal for a


It might be easier to visually scan for keyDown over key down.

Updated in rebased d29d5d1.

tofumatt · 2018-08-09T00:16:49Z

packages/editor/src/components/rich-text/index.js

@@ -426,41 +427,84 @@ export class RichText extends Component {
 	}

 	/**
-	 * Handles a keydown event from TinyMCE.
+	 * Handles a delete key down event to handle merge or removal for a
+	 * collapsed selection where caret is at directional edge: forward for a


An article (eg "the caret") here might improve readability.

Added link in rebased d29d5d1.

tofumatt · 2018-08-09T00:19:26Z

packages/editor/src/components/rich-text/index.js

-			if ( ! this.props.onMerge && ! this.props.onRemove ) {
-				return;
-			}
+		// Only process delete if occurs at uncollapsed edge.


Sorry to nitpick but this comment is a bit unclear... if what occurs? I guess this should be something like:

// Only process delete even if the deletion occurs at an uncollapsed edge.

Updated in rebased d29d5d1.

tofumatt · 2018-08-09T00:21:33Z

test/e2e/support/utils.js

@@ -77,6 +77,34 @@ async function login() {
 	] );
 }

+/**
+ * Returns a promise which resolves once it's determined that the active DOM


Thanks, this write-up is great 👍

aduth · 2018-08-09T14:26:16Z

Okay, the latest rebased iteration seems to handle all cases. The major change is only handling onRemove from RichText if the key pressed is the backspace key. This seems like the more correct interaction to expect for removal anyways, in my opinion.

tofumatt

I'm unclear about:

with a maximum of one removal

in the testing instructions. Does that mean the forward delete key shouldn't trigger multiple empty paragraph deletions? It seems like it should conceptually, and it does:

This works great though, otherwise tested well locally, felt good to use, and tests passed locally for me. I made a tiny tweak (keydown to keyDown in a comment because I thought it made it more readable as an event).

🚢

aduth · 2018-08-09T14:59:29Z

Does that mean the forward delete key shouldn't trigger multiple empty paragraph deletions?

Yep, for example, in the 3.4 release if you had multiple empty paragraphs, hitting (forward) Delete in the first of them would destroy two.

In the following recording, I hit Delete once, but two paragraphs are deleted (and the caret randomly moves to the preceding paragraph):

(Reiterating, this is recorded on 3.4, intended to be resolved with this pull request)

tofumatt · 2018-08-09T15:02:10Z

Oh wait, okay, I understand the instruction now. I just misinterpreted what you meant. Aces! 👍

aduth added 2 commits August 8, 2018 18:46

Testing: Wait for RichText initialization

25fb109

RichText: Fix empty value delete behaviors

1c58754

aduth added [Type] Bug An existing feature does not function as intended [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [Feature] Writing Flow Block selection, navigation, splitting, merging, deletion... labels Aug 8, 2018

aduth added this to the 3.5 milestone Aug 8, 2018

tofumatt reviewed Aug 9, 2018

View reviewed changes

ellatrix mentioned this pull request Aug 9, 2018

Rewrite table block to use a simpler RichText value #8767

Merged

4 tasks

RichText: Handle remove only on backspace

d29d5d1

tofumatt self-requested a review August 9, 2018 14:35

docs: Very minor comment readability tweak

eab0afa

tofumatt approved these changes Aug 9, 2018

View reviewed changes

aduth merged commit 45e3391 into master Aug 9, 2018

aduth deleted the fix/8731-empty-delete branch August 9, 2018 15:01

dependabot-preview bot mentioned this pull request Dec 31, 2019

Bump @wordpress/i18n from 2.0.0 to 3.7.0 wearerequired/rest-likes#97

Closed

dependabot-preview bot mentioned this pull request Feb 4, 2020

Bump @wordpress/jest-console from 3.4.0 to 3.5.0 tdreyno/druyan#215

Merged

dependabot-preview bot mentioned this pull request Apr 15, 2020

Bump @wordpress/babel-plugin-makepot from 3.4.0 to 3.5.0 swissspidy/gutenberg-i18n-block#93

Merged

aduth mentioned this pull request May 19, 2020

Buttons: allow split and merge #22436

Merged

6 tasks

ntsekouras mentioned this pull request Sep 23, 2020

Deleting empty paragraph also deletes proceeding block #19224

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RichText: Fix empty value delete behaviors #8735

RichText: Fix empty value delete behaviors #8735

aduth commented Aug 8, 2018 •

edited

Loading

aduth commented Aug 8, 2018 •

edited

Loading

tofumatt left a comment

tofumatt Aug 9, 2018

aduth Aug 9, 2018

tofumatt Aug 9, 2018

aduth Aug 9, 2018

tofumatt Aug 9, 2018

aduth Aug 9, 2018

tofumatt Aug 9, 2018

aduth Aug 9, 2018

tofumatt Aug 9, 2018

aduth Aug 9, 2018

tofumatt Aug 9, 2018

aduth commented Aug 9, 2018

tofumatt left a comment

aduth commented Aug 9, 2018 •

edited

Loading

tofumatt commented Aug 9, 2018

RichText: Fix empty value delete behaviors #8735

RichText: Fix empty value delete behaviors #8735

Conversation

aduth commented Aug 8, 2018 • edited Loading

aduth commented Aug 8, 2018 • edited Loading

tofumatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth commented Aug 9, 2018

tofumatt left a comment

Choose a reason for hiding this comment

aduth commented Aug 9, 2018 • edited Loading

tofumatt commented Aug 9, 2018

aduth commented Aug 8, 2018 •

edited

Loading

aduth commented Aug 8, 2018 •

edited

Loading

aduth commented Aug 9, 2018 •

edited

Loading